Add counterparty_skimmed_fee_msat field to PaymentKind::Bolt11Jit#497
Add counterparty_skimmed_fee_msat field to PaymentKind::Bolt11Jit#497tnull merged 2 commits intolightningdevkit:mainfrom
counterparty_skimmed_fee_msat field to PaymentKind::Bolt11Jit#497Conversation
tnull
force-pushed
the
2025-03-payment-details-nits
branch
from
ffad4f3 to
05c77da
Compare
| assert_eq!(hash, h); | ||
| assert_eq!(preimage, p); | ||
| assert_eq!(secret, s); | ||
| assert_eq!(None, c); |
There was a problem hiding this comment.
nit: should we test with Some() as well?
There was a problem hiding this comment.
No? This test checks that we're able to deserialize a (by now ancient) version of PaymentDetails. As we're just adding the new field, it always has to be None for any PaymentDetails deserialized from the old version.
There was a problem hiding this comment.
I understand this test is for old payments deser, but this comment was specifically if we should test with deser of new field in another test such as payment_info_is_persisted
There was a problem hiding this comment.
Hmm, the goal of payment_info_is_persisted is not to cover all possible edge cases and to re-test upstream's serialization macros. If we think we'd want to cover that, I'd rather write proptests for it, which is out-of-scope for this PR though.
src/payment/store.rs
Outdated
| preimage: Option<PaymentPreimage>, | ||
| /// The secret used by the payment. | ||
| secret: Option<PaymentSecret>, | ||
| /// The value, in thousands of a satoshi, that was deducted off of this payment as an extra |
There was a problem hiding this comment.
"The value, in thousands of a satoshi" is an awkward way of saying that the value is in msat?
where msat is thousandth of a satoshi.
| /// The value, in thousands of a satoshi, that was deducted off of this payment as an extra | |
| /// The value, in millisatoshis (msat), that was deducted off of this payment as an extra |
There was a problem hiding this comment.
Mhh, well the msat denomination is given in the variable name, no? These docs are copied from LDK, and tbh., since they are accurate, I don't see the need to change them here?
tnull
force-pushed
the
2025-03-payment-details-nits
branch
from
05c77da to
6297502
Compare
|
Excuse the delay here, now addressed the feedback. |
G8XSU
left a comment
G8XSU
left a comment
There was a problem hiding this comment.
Lgtm!
Please feel free to squash fixups.
| assert_eq!(hash, h); | ||
| assert_eq!(preimage, p); | ||
| assert_eq!(secret, s); | ||
| assert_eq!(None, c); |
There was a problem hiding this comment.
I understand this test is for old payments deser, but this comment was specifically if we should test with deser of new field in another test such as payment_info_is_persisted
.. we add a field describing exactly how much the LSP withheld.
tnull
force-pushed
the
2025-03-payment-details-nits
branch
from
6297502 to
13c6d0e
Compare
Squashed without further changes. |
2 participants
Closes #495
We add a field to
Bolt11Jitdescribing exactly how much the LSP withheld.Additionally, we clarify when
PaymentDetails::amount_msatmight beNone(for now).